pkg/postgres/roles.go (3)

164-188: Optimize repeated subquery for role OID lookup.

The subquery (SELECT oid FROM pg_roles WHERE rolname = $1) is executed seven times within the UNION ALL, which is inefficient. Consider using a CTE to fetch the OID once and reference it in all subsequent checks.

Apply this diff to optimize the query:

 	query := `
-		SELECT EXISTS(
-			SELECT 1 FROM (
-				-- Check for owned schemas
-				SELECT 1 FROM pg_namespace WHERE nspowner = (SELECT oid FROM pg_roles WHERE rolname = $1)
-				UNION ALL
-				-- Check for owned tables
-				SELECT 1 FROM pg_class WHERE relowner = (SELECT oid FROM pg_roles WHERE rolname = $1)
-				UNION ALL
-				-- Check for owned functions
-				SELECT 1 FROM pg_proc WHERE proowner = (SELECT oid FROM pg_roles WHERE rolname = $1)
-				UNION ALL
-				-- Check for owned sequences
-				SELECT 1 FROM pg_class WHERE relowner = (SELECT oid FROM pg_roles WHERE rolname = $1) AND relkind = 'S'
-				UNION ALL
-				-- Check for owned views
-				SELECT 1 FROM pg_class WHERE relowner = (SELECT oid FROM pg_roles WHERE rolname = $1) AND relkind = 'v'
-				UNION ALL
-				-- Check for owned types
-				SELECT 1 FROM pg_type WHERE typowner = (SELECT oid FROM pg_roles WHERE rolname = $1)
-				UNION ALL
-				-- Check for owned databases
-				SELECT 1 FROM pg_database WHERE datdba = (SELECT oid FROM pg_roles WHERE rolname = $1)
-			) owned_objects
-		)`
+		WITH role_oid AS (
+			SELECT oid FROM pg_roles WHERE rolname = $1
+		)
+		SELECT EXISTS(
+			SELECT 1 FROM (
+				-- Check for owned schemas
+				SELECT 1 FROM pg_namespace WHERE nspowner = (SELECT oid FROM role_oid)
+				UNION ALL
+				-- Check for owned tables, sequences, views, etc.
+				SELECT 1 FROM pg_class WHERE relowner = (SELECT oid FROM role_oid)
+				UNION ALL
+				-- Check for owned functions
+				SELECT 1 FROM pg_proc WHERE proowner = (SELECT oid FROM role_oid)
+				UNION ALL
+				-- Check for owned types
+				SELECT 1 FROM pg_type WHERE typowner = (SELECT oid FROM role_oid)
+				UNION ALL
+				-- Check for owned databases
+				SELECT 1 FROM pg_database WHERE datdba = (SELECT oid FROM role_oid)
+			) owned_objects
+		)`

---

329-370: Consider wrapping operations in a transaction.

The safe deletion process involves multiple database operations (checking ownership, revoking grants, removing memberships, dropping the role). If any step fails midway, the database is left in an inconsistent state. While the operations are idempotent and can be retried, wrapping them in a transaction would ensure atomicity.

Note: PostgreSQL DDL statements like REVOKE and DROP ROLE are transactional, so a transaction would provide rollback capability if any step fails.

Example structure:

func (c *Client) SafeDeleteRole(ctx context.Context, roleName string) error {
	l := ctxzap.Extract(ctx)

	if roleName == "" {
		return errors.New("role name cannot be empty")
	}

	// Start transaction
	tx, err := c.db.Begin(ctx)
	if err != nil {
		return err
	}
	defer tx.Rollback(ctx)

	// Perform operations using tx instead of c.db
	// ... ownership check, revoke grants, remove memberships, drop role ...

	// Commit transaction
	return tx.Commit(ctx)
}

However, implementing this requires refactoring RevokeAllGrantsFromRole and RemoveRoleFromAllRoles to accept a transaction or executor interface.

---

200-276: Consider revoking DEFAULT PRIVILEGES.

The current implementation revokes direct grants on existing objects but does not revoke DEFAULT PRIVILEGES. If the role has DEFAULT PRIVILEGES grants (which apply to future objects), those should also be revoked before deletion.

Add this after line 273:

 	}
 
+	// Revoke default privileges
+	for _, schema := range schemas {
+		sanitizedSchema := pgx.Identifier{schema}.Sanitize()
+		revokeDefaultQuery := fmt.Sprintf("ALTER DEFAULT PRIVILEGES FOR ROLE %s IN SCHEMA %s REVOKE ALL ON TABLES FROM %s", sanitizedRoleName, sanitizedSchema, sanitizedRoleName)
+		l.Debug("revoking default table privileges", zap.String("query", revokeDefaultQuery))
+		if _, err := c.db.Exec(ctx, revokeDefaultQuery); err != nil {
+			l.Error("error revoking default table privileges", zap.String("schema", schema), zap.Error(err))
+			return err
+		}
+		// Repeat for SEQUENCES, FUNCTIONS, TYPES as needed
+	}
+
 	revokeDbQuery := fmt.Sprintf("REVOKE ALL ON DATABASE %s FROM %s", pgx.Identifier{c.DatabaseName()}.Sanitize(), sanitizedRoleName)

Note: This requires checking which roles the target role granted DEFAULT PRIVILEGES for, which may require querying pg_default_acl.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

pkg/postgres/roles.go (1)

174-183: Remove redundant sequence and view checks.

Lines 180 and 183 duplicate the check on Line 174, which already covers all pg_class objects (tables, sequences, views, etc.) via the relowner filter. The separate UNION ALL clauses for sequences (relkind = 'S') and views (relkind = 'v') are unnecessary.

Based on learnings

Apply this diff to remove the redundant checks:

 		SELECT EXISTS(
 			SELECT 1 FROM (
 				-- Check for owned schemas
 				SELECT 1 FROM pg_namespace WHERE nspowner = (SELECT oid FROM pg_roles WHERE rolname = $1)
 				UNION ALL
 				-- Check for owned tables
 				SELECT 1 FROM pg_class WHERE relowner = (SELECT oid FROM pg_roles WHERE rolname = $1)
 				UNION ALL
 				-- Check for owned functions
 				SELECT 1 FROM pg_proc WHERE proowner = (SELECT oid FROM pg_roles WHERE rolname = $1)
 				UNION ALL
-				-- Check for owned sequences
-				SELECT 1 FROM pg_class WHERE relowner = (SELECT oid FROM pg_roles WHERE rolname = $1) AND relkind = 'S'
-				UNION ALL
-				-- Check for owned views
-				SELECT 1 FROM pg_class WHERE relowner = (SELECT oid FROM pg_roles WHERE rolname = $1) AND relkind = 'v'
-				UNION ALL
 				-- Check for owned types
 				SELECT 1 FROM pg_type WHERE typowner = (SELECT oid FROM pg_roles WHERE rolname = $1)
 				UNION ALL
 				-- Check for owned databases
 				SELECT 1 FROM pg_database WHERE datdba = (SELECT oid FROM pg_roles WHERE rolname = $1)
 			) owned_objects
 		)`

pkg/postgres/roles.go (1)

395-397: Clarify the error handling intent.

The errors.Is check on Line 395 prevents early return when grantsRevokeError is wrapped with errRevokeGrantsFromRole. Since RevokeAllGrantsFromRole always wraps errors with this sentinel (Line 311), this condition will never be true. If the intent is to always continue despite grant revocation failures and report them at the end, consider simplifying or adding a comment to clarify this behavior.

Consider simplifying to:

 	l.Debug("revoking all grants from role", zap.String("role", roleName))
 	grantsRevokeError := c.RevokeAllGrantsFromRole(ctx, roleName)
 	if grantsRevokeError != nil {
 		l.Error("error revoking grants from role", zap.Error(grantsRevokeError))
-		if !errors.Is(grantsRevokeError, errRevokeGrantsFromRole) {
-			return fmt.Errorf("error revoking existing grants from role: %w", grantsRevokeError)
-		}
+		// Continue to attempt role deletion; errors will be joined at the end if DROP ROLE fails
 	}

Or add a comment explaining the intent if you prefer to keep the check.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro